Repository navigation
Conversation
Maddily
left a comment
There was a problem hiding this comment.
Left small suggestions throughout: typo fixes, grammar, a couple of technical corrections, and some redundant phrasing tightened for clarity. Nothing structural, just polish. Great lesson overall!
|
@Maddily Thanks a lot for having taken a much closer look and scrutinize this lesson! Since commit history is getting pretty long, I'll just squash them first locally and push to make the history look more succinct :) |
7e6292b to
b389234
Compare
mao-sz
left a comment
There was a problem hiding this comment.
This is looking great! Got some initial comments, some small fixes and some larger conceptual ideas. As always, feel free to push back on anything if you feel justified.
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
It was being too much "if you do this everything will be okay" Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Co-authored-by: Goran Jović <goran1010jovic@gmail.com>
|
Curious as to why the linter passes here when the branch hasn't been synced with main yet (the markdownlint workflow runs on |
@mao-sz https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#how-the-merge-branch-affects-your-workflow - this? |
Co-authored-by: Goran Jović <goran1010jovic@gmail.com>
Co-authored-by: Goran Jović <goran1010jovic@gmail.com>
mao-sz
left a comment
There was a problem hiding this comment.
Content on CORS is now going to come in the JSON Web Tokens lesson, so no need to include it in this anymore.
Co-authored-by: Goran Jović <goran1010jovic@gmail.com>
| ### Cross-Site Request Forgery | ||
|
|
||
| Another very common exploit in the World Wide Web. Imagine you encountered a strange but very innocent looking website called `cutekittens.com`...until you inspect its HTML that looks like this. | ||
|
|
||
| ```html | ||
| <form action="https://my-bank.example.org/transfer" method="POST"> | ||
| <input type="hidden" name="recipient" value="attacker" /> | ||
| <input type="hidden" name="amount" value="1000" /> | ||
| </form> | ||
| ``` | ||
|
|
||
| Then, within that site, a small but very dangerous snippet of JavaScript is embedded and will be loaded if anyone clicked on it... | ||
|
|
||
| ```javascript | ||
| const form = document.querySelector("form"); | ||
| form.submit(); | ||
| ``` | ||
|
|
||
| Assuming you are still logged in and your banking app's cookies still persist, disaster has already been struck! "Wait, I just want to see kittens! Why is my money gone?!" Yikes, you don't want that, right? [Cross-Site Request Forgery (CSRF) attacks](https://developer.mozilla.org/en-US/docs/Web/Security/Attacks/CSRF) are just that: tricking a user into performing a state-changing request to a certain target site from a malicious site. In this case, a form was automatically submit the moment you landed on `cutekittnes.com` and a banking transaction was silently performed. | ||
|
|
||
| So what could you do about it? One of the most common methods is to use a CSRF token. Every time you prompt your user to perform some state-changing request in your application, check for a hidden, unpredictable key (this could be embedded inside a `<form>` or an HTTP request header). Then, in your app, carry out the request if and only if the token is valid. |
There was a problem hiding this comment.
One thing I think makes this section incomplete is that it's not quite clear exactly where the learner stands in relation to all this.
e.g. from this section, I can absolutely see a learner going "how would I use a CSRF token here? I don't have anything to do with cutekittens.com nor my-bank.example.org!"
The example used here makes sense but it's the bank site's responsibility to implement anti-CSRF measures, not the user's responsibility.
This section needs to clearly frame things so that it's relevant to the learner and them building sites/servers. The problem is specifically that if they're building something, not using anti-CSRF techniques like CSRF tokens is what can allow a malicious site to make a malicious request. By using something like CSRF tokens, a malicious site can certainly try a CSRF attack on your site but since they won't have the correct token, the request will not succeed.
So this example can be kept but instead of saying "So what could you do about it?" (which makes it sound like the user is the one to protect the banking site), frame it as what the banking site could do to protect against CSRF.
Then expand by framing things for when the learner is making their own sites, and how they can use stuff like CSRF tokens to protect users of their sites from CSRF.
| ### Cross-Site Request Forgery | ||
|
|
||
| Another very common exploit in the World Wide Web. Imagine you encountered a strange but very innocent looking website called `cutekittens.com`...until you inspect its HTML that looks like this. | ||
|
|
||
| ```html | ||
| <form action="https://my-bank.example.org/transfer" method="POST"> | ||
| <input type="hidden" name="recipient" value="attacker" /> | ||
| <input type="hidden" name="amount" value="1000" /> | ||
| </form> | ||
| ``` | ||
|
|
||
| Then, within that site, a small but very dangerous snippet of JavaScript is embedded and will be loaded if anyone clicked on it... | ||
|
|
||
| ```javascript | ||
| const form = document.querySelector("form"); | ||
| form.submit(); | ||
| ``` | ||
|
|
||
| Assuming you are still logged in and your banking app's cookies still persist, disaster has already been struck! "Wait, I just want to see kittens! Why is my money gone?!" Yikes, you don't want that, right? [Cross-Site Request Forgery (CSRF) attacks](https://developer.mozilla.org/en-US/docs/Web/Security/Attacks/CSRF) are just that: tricking a user into performing a state-changing request to a certain target site from a malicious site. In this case, a form was automatically submit the moment you landed on `cutekittnes.com` and a banking transaction was silently performed. | ||
|
|
||
| So what could you do about it? One of the most common methods is to use a CSRF token. Every time you prompt your user to perform some state-changing request in your application, check for a hidden, unpredictable key (this could be embedded inside a `<form>` or an HTTP request header). Then, in your app, carry out the request if and only if the token is valid. |
There was a problem hiding this comment.
A separate thing I want to also point out about this section is that it probably won't be clear to learners exactly where the bank site URL comes from. The point is that whoever made the malicious site knows the URL (and params) that could get them what they want, and that's why they crafted the request URL in that way (see https://stackoverflow.com/a/33829607 for more context). It's not just any ol' URL/request to some random site.
|
|
||
| Eventually, you might want to adapt your app to a wider range of audiences around the world. [Internationalization (I18n)](https://learn.microsoft.com/en-us/globalization/methodology/software-internationalization) is a process of adapting your applications to different populations/languages. This could involve taking a look at a client's [Accept-Language header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept-Language) and serve the content in the language that they want to see. | ||
|
|
||
| MDN documentations also accommodate a wide range of languages! For example, <https://developer.mozilla.org/fr/> will display all your pages in MDN in French! C'est pratique, non? |
There was a problem hiding this comment.
- "documentation" is an uncountable noun (i.e. no plural)
- Rephrasing for better flow from the previous line. Starting straight with "MDN also accommodates..." reads to me a little like it's out of nowhere.
| MDN documentations also accommodate a wide range of languages! For example, <https://developer.mozilla.org/fr/> will display all your pages in MDN in French! C'est pratique, non? | |
| For example, MDN can accommodate a wide range of languages! For example, <https://developer.mozilla.org/fr/> will display all your pages in MDN in French! C'est pratique, non? |
|
|
||
| Eventually, you might want to adapt your app to a wider range of audiences around the world. [Internationalization (I18n)](https://learn.microsoft.com/en-us/globalization/methodology/software-internationalization) is a process of adapting your applications to different populations/languages. This could involve taking a look at a client's [Accept-Language header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept-Language) and serve the content in the language that they want to see. | ||
|
|
||
| MDN documentations also accommodate a wide range of languages! For example, <https://developer.mozilla.org/fr/> will display all your pages in MDN in French! C'est pratique, non? |
There was a problem hiding this comment.
What do you think about something like this:
| MDN documentations also accommodate a wide range of languages! For example, <https://developer.mozilla.org/fr/> will display all your pages in MDN in French! C'est pratique, non? | |
| For example, MDN can accommodate a wide range of languages! [MDN maintains a lot of translated content for various languages](https://github.com/mdn/translated-content), including [French](https://developer.mozilla.org/fr/ "MDN docs in French") and [German](https://developer.mozilla.org/de/ "MDN docs in German"). |
I think it's a good idea to make it clear that internationalisation is something that has to be manually implemented and not some automatic translation that can be done for any site by just adding headers. While the current text kinda hints at that, actually linking MDN's translated-content repo should make it much clearer that it's a pretty damn big job that has to be manually worked on (they only provide content for certain languages too, not just automatic translation for anything).
Also providing more than one language example feels nicer IMO (but that's just a nit; also using our style guide's aria-label/title links syntax for visually nicer links while still remaining sufficiently accessible.
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Co-authored-by: mao-sz <122839503+mao-sz@users.noreply.github.com>
Because
This new lesson is part of the Node Revamp project to be added to introduce learners to more advanced concepts in web development that learners will do more exploring on their own.
This PR
Issue
Closes #29531
Additional Information
Pull Request Requirements
location of change: brief description of changeformat, e.g.Intro to HTML and CSS lesson: Fix link textBecausesection summarizes the reason for this PRThis PRsection has a bullet point list describing the changes in this PRIssuesection